VableArrayStore - #1152
Conversation
WalkthroughThe change adds structured virtualizable store results, preserves overwritten shadow entries during snapshot capture, and supports reverse-indexed guard resume stamping. Runtime handlers and tests now use ChangesVirtualizable snapshot and guard handling
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant VableStore
participant TraceCtx
participant SnapshotCapture
participant Trace
VableStore->>TraceCtx: return VableArrayStore
TraceCtx->>SnapshotCapture: provide overwritten virtualizable entry
SnapshotCapture->>Trace: stamp emitted guards from the end
Trace->>SnapshotCapture: publish resume snapshots
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…every guard one opcode emits `capture_vable_promote_guard` ran after the whole `vable_*` call returned. `publish_last_guard_resume_snapshot` clones `ctx.virtualizable_boxes`, and by then the standard leg of `vable_setfield` / `vable_setarrayitem_indexed` has already written the slot -- so the promote guard's resume data carried the very write its own resume pc re-executes. `_opimpl_setfield_vable` / `_opimpl_setarrayitem_vable` (pyjitpl.py:1188 / :1236) reach `virtualizable_boxes[index] = valuebox` only after the promotes have captured. Both setters now report the slot they overwrote (`VableEntryWrite`) and the capture puts it back for its duration through `swap_virtualizable_entry`, which leaves `virtualizable_live_null_slots` alone where the store clears it and the `live_null_push` arm sets it right after. One vable array access can also emit two guards: the `isstandard` PTR_EQ in `_nonstandard_virtualizable` (:1135-1138) and then the index in `_get_arrayitem_vable_index` (:1201-1216). Only the last was stamped, so the first kept the `UNSTAMPED_JITCODE_INDEX` frame `record_guard_with_snapshot` mints. `set_guard_op_resume_position_from_end` walks back over every guard the call added; `set_last_guard_op_resume_position` is now its `from_end == 0` case. Two unit tests, each checked to fail with its half of the change reverted: the setarrayitem promote guard's snapshot holds the pre-store Box and not the value written, and both guards of a two-promote opcode point at a stamped frame. Assisted-by: Claude
The comment on `native_exact_str_replay` cited three compile aborts. Dropping the gate and rebuilding the guest reads `abrt_bad_loop=1` on `str_search_index_bounds`, with `bridges_compiled` 7 -> 5 and `guard_failures` 2096 -> 7098 against 1897 for the native backends, so the boundary stays. Also records what the boundary itself costs on that bench: the hazardous-callee arm of `fbw_abort_nested_unjournaled_residual` declines once, and the guard it leaves unbridged re-fires 199 more times, one `trace_eagerness` cycle later. Forcing the gate on for pyre-dynasm reproduces the recorded wasm row exactly (5 loops / 7 bridges / 1 abort / 2096 guard failures against 5 / 7 / 0 / 1897). Assisted-by: Claude
…he four callers the enum broke `capture_vable_promote_guard` walked `0..minted`, newest guard first. `publish_last_guard_resume_snapshot` is not side-effect free -- it leaves the root frame's in-flight result slot cleared (`root_inflight_int_result`) -- so the order decides what the earlier guard's snapshot sees. `generate_guard` (pyjitpl.py:2582-2603) captures each guard synchronously as it is emitted, so the loop runs oldest-first. The `bool -> VableArrayStore` return also broke four `assert!` call sites in `pyre-jit-trace`'s test module, which a release binary build and `cargo test -p majit-metainterp` both compile past; `cargo check --workspace --all-targets` is what reports them. `VableArrayStore` is re-exported from the crate root so the external test can name it. The doc comment claimed pyre's two snapshots match what upstream would produce. They do not: `MetaInterp.replace_box` walks the framestack (`frame.replace_active_box_in_frame`) and `TraceCtx::replace_box` does not, so upstream's second capture would see the standard box. Stated as the pre-existing gap it is. Assisted-by: Claude
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit 89192db). Files in the reviewed diff1. Regressions to PyPy parity introduced by this patchNone. 2. Other mismatches introduced by this patchNone. 3. Pre-existing mismatches (already present before this patch)
4. Structural adaptations
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 89192dbce4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
| self.synchronize_virtualizable(); | ||
| true | ||
| VableArrayStore::Stored(overwritten) |
There was a problem hiding this comment.
When a standard virtualizable store emits a promote guard, the production pyre-jit-trace handlers still discard this Stored(overwritten) value (vable_ops.rs:813) and call walker_capture_inline_nonstandard_vable_guard only after the shadow was updated (vable_ops.rs:839); vable_setfield is discarded similarly at lines 334-347. Thus, on a dynamic-index guard failure, resume data still restores the traced write into its old slot before re-executing the setter at the runtime index, potentially modifying two slots. The rollback token must be threaded into the production snapshot capture, not only the majit-metainterp test dispatcher.
AGENTS.md reference: AGENTS.md:L14-L19
Useful? React with 👍 / 👎.
| /// data inside each `implement_guard_value`, so both are stamped; a caller | ||
| /// that only reaches the guards after the helper returns walks back over | ||
| /// them with this. | ||
| pub fn set_guard_op_resume_position_from_end(&mut self, from_end: usize, snapshot_id: i32) { |
There was a problem hiding this comment.
For a pointer-equal but non-identical virtualizable with a dynamic array index, one access emits both the isstandard and index guards, but the production helper walker_capture_inline_nonstandard_vable_guard still calls only the capture_snapshot_for_last_guard_* variants (resume_snapshot.rs:166-173,212-219,246-253). Consequently only the index guard is restamped and the earlier guard retains its UNSTAMPED_JITCODE_INDEX placeholder, so a retained/failing identity guard cannot be decoded correctly. Wire this new indexed guard-stamping API into that production helper and capture every guard emitted by the opcode.
AGENTS.md reference: AGENTS.md:L231-L233
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
majit/majit-metainterp/src/pyjitpl.rs (1)
5089-5108: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winHandle
VableArrayStore::OutOfVableinstead of assertingStored.
vable_setarrayitem_indexedreturnsOutOfVablefor an out-of-range virtualizable index. This is a valid trace-abort outcome, not a missing-slot invariant violation. The assertions in all threeopimpl_setarrayitem_vable_*methods can panic before the caller handles the abort.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@majit/majit-metainterp/src/pyjitpl.rs` around lines 5089 - 5108, Update all three opimpl_setarrayitem_vable_* methods to handle VableArrayStore::OutOfVable as the valid trace-abort result from vable_setarrayitem_indexed, rather than asserting Stored. Preserve assertion or invariant handling only for genuinely unexpected results, and allow the existing caller abort path to process out-of-range indices.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pyre/pyre-jit-trace/src/jitcode_dispatch/tests.rs`:
- Around line 604-617: In the repeated-store assertions around
vable_setarrayitem_indexed, tighten the VableArrayStore::Stored pattern from
Stored(_) to Stored(Some(_)) at both referenced assertion sites. Preserve the
existing setup and verify that each overwrite reports a previous shadow entry
rather than accepting Stored(None).
---
Outside diff comments:
In `@majit/majit-metainterp/src/pyjitpl.rs`:
- Around line 5089-5108: Update all three opimpl_setarrayitem_vable_* methods to
handle VableArrayStore::OutOfVable as the valid trace-abort result from
vable_setarrayitem_indexed, rather than asserting Stored. Preserve assertion or
invariant handling only for genuinely unexpected results, and allow the existing
caller abort path to process out-of-range indices.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: aab67c93-a175-47cc-95b1-0dfa51f1dc28
📒 Files selected for processing (8)
majit/majit-metainterp/src/history.rsmajit/majit-metainterp/src/lib.rsmajit/majit-metainterp/src/pyjitpl.rsmajit/majit-metainterp/src/pyjitpl/dispatch.rsmajit/majit-metainterp/src/recorder.rsmajit/majit-metainterp/src/trace_ctx.rspyre/pyre-jit-trace/src/jitcode_dispatch/residual_call.rspyre/pyre-jit-trace/src/jitcode_dispatch/tests.rs
| assert!(matches!( | ||
| tc.vable_setarrayitem_indexed( | ||
| 0, | ||
| vable, | ||
| index0, | ||
| 0, | ||
| fdescr.clone(), | ||
| adescr.clone(), | ||
| const_null, | ||
| null, | ||
| false, | ||
| ), | ||
| VableArrayStore::Stored(_) | ||
| )); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Assert the overwritten payload.
VableArrayStore::Stored(_) also matches VableArrayStore::Stored(None). Lines 604-617 and 638-641 run after earlier writes to flat_base, so these assertions do not prove that VableEntryWrite::of returned the previous shadow entry. A regression that drops that entry can still pass this test. Require VableArrayStore::Stored(Some(_)) for these repeated stores.
Proposed assertion tightening
- VableArrayStore::Stored(_)
+ VableArrayStore::Stored(Some(_))Also applies to: 638-641
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pyre/pyre-jit-trace/src/jitcode_dispatch/tests.rs` around lines 604 - 617, In
the repeated-store assertions around vable_setarrayitem_indexed, tighten the
VableArrayStore::Stored pattern from Stored(_) to Stored(Some(_)) at both
referenced assertion sites. Preserve the existing setup and verify that each
overwrite reports a previous shadow entry rather than accepting Stored(None).
1 participant
Summary by CodeRabbit
Bug Fixes
Tests
Documentation